-
Notifications
You must be signed in to change notification settings - Fork 99
Adds call_object_method. #2195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Adds call_object_method. #2195
Conversation
| """ | ||
| Calls a method on an object with the supplied arguments. | ||
|
|
||
| This method allows us to construct a callable method for DAGRunner to execute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't reference what library you choose to execute these with.
| This method allows us to construct a callable method for DAGRunner to execute | |
| This method allows us to construct a callable method for the caller to execute |
|
Neat idea. This would avoid a proliferation of applications that do non-science execution. method_name = kwargs.pop('obj_method')
method = getattr(obj, method_name)
return method(**kwargs)In fact, I would say that it could be useful to extend this idea to passing multiple arguments (cubes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to see about making this handle multiple arguments as in the suggested changes. This way, also calling a CubeList method instead of a cube method where applicable. This will let you do things like turning multiple cubes/CubeList inputs into a single cube (merge_cube call).
| """module to give access to callable methods on objects.""" | ||
|
|
||
|
|
||
| def call_object_method(obj: object, method_name: str, **kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def call_object_method(obj: object, method_name: str, **kwargs): | |
| def call_cubelike_method(*cubes: Union[Cube, CubeList], method_name: str = None, **kwargs): -> Union[Cube, CubeList] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was that we did not have to be restricted to cubelike objects. ANY object could be used in this way, e.g. a NumPy array or a Pandas dataframe.
I am wondering whether this would be better as a Plugin class though, with the init method taking the name of the attribute to be called, so that args and kwargs can be disambiguated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need that degree of generality, especially given the benefit we might receive from assuming pipelines using cubes and cubelists.
(improver applications right now expect cubes/cubelists)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also realised that we don't need this at all. For any class object, you can do MyClass.method(class_object), so iris.cube.Cube.collapsed(temperature_cube, "height", iris.analysis.MAX) would give the same answer as temperature_cube.collapsed("height", iris.analysis.MAX), so I don't think this PR adds anything we can't already do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, but remaining benefit as I see it is in this handling of 1 or more argument (Cube or CubeList) in a way that simplifies handling of such method calls in the workflow (so you don't need an additional step to handle inputs).
Illustration only:
graph LR;
proc_C["call_cubelike_method(<br>collapsed, ...)"]
proc_A --> proc_C;
proc_B --> proc_C;
VS
graph LR;
proc_A --> merge_cube;
proc_B --> merge_cube;
merge_cube --> collapsed[iris.cube.Cube.collapsed]
OR
Wrapping every possible Cubes and CubeList method with its own plugin to handle the more than 1 input cube/cubelist.
Just a thought -- not saying definitively what the right thing to do is here -- certainly a class plugin would be the way to distinguish between input object handling (plugin __init__) and the calling of the resulting underlying method call (__call__).
| method = getattr(obj, method_name) | ||
| return method(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| method = getattr(obj, method_name) | |
| return method(**kwargs) | |
| if len(cubes) > 1: | |
| cubes = as_cubelist(*cubes) | |
| method = getattr(cubes, method_name) | |
| return method(**kwargs) |
| # This file is part of 'IMPROVER' and is released under the BSD 3-Clause license. | ||
| # See LICENSE in the root of the repository for full licensing details. | ||
|
|
||
| """module to give access to callable methods on objects.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| """module to give access to callable methods on objects.""" | |
| """module to give access to callable methods on objects.""" | |
| from iris.cube import Cube, CubeList | |
| from improver.utilities.common_input_handle import as_cubelist |
On a few occasions, I've found a need to write a wrapper for a specific class method, e.g.
merge_cube()for aCubeListobject, orcollapsedfor aCubeobject. Our current methodology requires us calling a method and passing an object into it, rather than calling a method on an object, hence the need for this simple wrapper function.I have deliberately removed the ability to pass additional positional args as I think allowing this would make the calls to this method much harder to understand.
Testing: